Skip to content

Added default connect_timeout in cluster config#16453

Merged
phlax merged 8 commits intoenvoyproxy:mainfrom
mk46:comment_required
May 20, 2021
Merged

Added default connect_timeout in cluster config#16453
phlax merged 8 commits intoenvoyproxy:mainfrom
mk46:comment_required

Conversation

@mk46
Copy link
Copy Markdown
Contributor

@mk46 mk46 commented May 12, 2021 •
edited by phlax
Loading

Signed-off-by: Manish Kumar manish.kumar1@india.nec.com

Added default value of 5s for connect_timeout in the cluster config

Fixes #16439

Fixes #16583

Fixed missing required in cluster config for connect_timeout
465d95a 
Signed-off-by: Manish Kumar <manish.kumar1@india.nec.com>
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented May 12, 2021

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16453 was opened by mk46.

see: more, trace.

@phlax
Copy link
Copy Markdown
Member

phlax commented May 12, 2021

@mk46 looks good, but i would like to check that it really is required

afaict from quick grep all of the config has this setting - so it seems like it is required as @daixiang0 has found - i would like to confirm this 100%

the other thing im wondering is if it isnt marked required already in the api - is the problem with the (api) docs or should it have some default in the actual code

cc @htuch

fix format.
e458506 
Signed-off-by: Manish Kumar <manish.kumar1@india.nec.com>
@phlax
Copy link
Copy Markdown
Member

phlax commented May 12, 2021

@mk46 not sure why exactly - but looks like CI doesnt like this change

@mk46
Copy link
Copy Markdown
Contributor Author

mk46 commented May 12, 2021

I had fixed the CI error in CdsApiImplTest but not sure about ExampleConfigsTest.

@phlax
Copy link
Copy Markdown
Member

phlax commented May 12, 2021

not 100% sure but looking here https://dev.azure.com/cncf/envoy/_build/results?buildId=75433&view=logs&j=8c169225-0ae8-53bd-947f-07cb81846cb5&t=6b0ace90-28b2-51d9-2951-b1fd35e67dec&l=21656

...it looks to me like making this required is also enforcing for xDS (eds) and not just bootstrap config - afaict whatever the situation with bootstrap it looks as though its not currently required for non-boostrap config of clusters

@phlax phlax self-assigned this May 12, 2021
Manish Kumar added 3 commits May 13, 2021 00:22
fix
0a722b1 
Signed-off-by: Manish Kumar <manish.kumar1@india.nec.com>
Added default value 5s to connect_timeout
92376b6 
Signed-off-by: Manish Kumar <manish.kumar1@india.nec.com>
fix
e3dfc08 
Signed-off-by: Manish Kumar <manish.kumar1@india.nec.com>
@mk46 mk46 changed the title Fixed missing "required" in cluster config for connect_timeout Added default connect_timeout in cluster config May 12, 2021
@mk46
Copy link
Copy Markdown
Contributor Author

mk46 commented May 12, 2021

@phlax @htuch Please take a look.

@lizan
Copy link
Copy Markdown
Member

lizan commented May 12, 2021

Can this be tested somehow?

Added unit test for checking default value.
b0b5888 
Signed-off-by: Manish Kumar <manish.kumar1@india.nec.com>
@mk46
Copy link
Copy Markdown
Contributor Author

mk46 commented May 12, 2021

Hi @lizan, I have added a unit test for checking the default value. Please take a look.

@mk46
Copy link
Copy Markdown
Contributor Author

mk46 commented May 13, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented May 13, 2021

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16453 (comment) was created by @mk46.

see: more, trace.

@mk46
Copy link
Copy Markdown
Contributor Author

mk46 commented May 13, 2021

I am not sure why GCC is failing, which is not related to current changes.

@phlax
Copy link
Copy Markdown
Member

phlax commented May 13, 2021

I am not sure why GCC is failing, which is not related to current changes.

its a problem on main - its being worked on here #16484

@mk46
Copy link
Copy Markdown
Contributor Author

mk46 commented May 14, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented May 14, 2021

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16453 (comment) was created by @mk46.

see: more, trace.

htuch
htuch approved these changes May 14, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

wrowe
wrowe previously requested changes May 19, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is solid, but missing docs of the new default. Perhaps just a tracking issue to ensure the docs are refreshed if this is merged first?

https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/cluster/v3/cluster.proto.html
https://www.envoyproxy.io/docs/envoy/latest/faq/configuration/timeouts

@phlax phlax mentioned this pull request May 19, 2021
Closed
htuch
htuch previously approved these changes May 19, 2021
View reviewed changes
htuch
htuch reviewed May 20, 2021
View reviewed changes
Manish Kumar added 2 commits May 20, 2021 15:29
Merge remote-tracking branch 'upstream/main' into comment_required
4322852 
Signed-off-by: Manish Kumar <manish.kumar1@india.nec.com>
Added docs in faq and version_history.
f0928f2 
Signed-off-by: Manish Kumar <manish.kumar1@india.nec.com>
@phlax
Copy link
Copy Markdown
Member

phlax commented May 20, 2021

docs rendered here https://storage.googleapis.com/envoy-pr/16453/docs/index.html

phlax
phlax approved these changes May 20, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - thanks @mk46

you might need to kick ci as it seems stuck

@phlax phlax dismissed wrowe’s stale review May 20, 2021 13:24

requested changes have been applied

@phlax phlax merged commit 5218436 into envoyproxy:main May 20, 2021
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Added default connect_timeout in cluster config (envoyproxy#16453)
6a78f6f 
Signed-off-by: Manish Kumar <manish.kumar1@india.nec.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phlax phlax phlax approved these changes

+2 more reviewers

@htuch htuch htuch left review comments

@wrowe wrowe wrowe left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@phlax phlax

@lizan lizan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

proto validation: Cluster message should require "connect_timeout" field be set Required field not comment in docs

5 participants

@mk46 @phlax @lizan @wrowe @htuch